-
Notifications
You must be signed in to change notification settings - Fork 401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: reproduce dune exec -w
crash with pkg management
#10960
base: main
Are you sure you want to change the base?
Conversation
c437ca5
to
deb6397
Compare
is initialising when this happens so I need to find who is updating the build progress this early. |
For some reason this stems from the |
Signed-off-by: Antonio Nuno Monteiro <[email protected]>
deb6397
to
dcc55f4
Compare
Signed-off-by: Antonio Nuno Monteiro <[email protected]>
I just pushed a commit that delays evaluating |
Signed-off-by: Antonio Nuno Monteiro <[email protected]>
|
||
$ dune exec -w ./x.exe > output.log 2>&1 & | ||
$ PID=$! | ||
$ sleep 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2
seems long, what about 0.1
? The other exec-watch
tests appear to pick something like that. Although killing processes this was seems very fragile, there ought to be a more principled way to test these. Not this PRs problem however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The watching/ tests use some better helpers. Up to you if you want to improve things. Also I would move this from pkg
to exec-watch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you point me to a relevant example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have these start_dune
stop_dune
helpers from a shared script (see the dune file for where it is). A test example is: https://github.com/ocaml/dune/blob/main/test/blackbox-tests/test-cases/watching/copy-rules.t
The logic in the script handles stopping the watch mode build and the output. Presumably something like that could be adapted for exec
. But if this is too much work that is also fine, since the other exec-watch/
tests are written this way.
@@ -69,10 +69,9 @@ module Command_to_exec = struct | |||
|
|||
type t = | |||
{ get_path_and_build_if_necessary : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps call this get_env_and_build_if_necessary
.
before addressing the outstanding code review items, I'd like some confirmation that the fix is in the right direction, or even correct. |
reproduces the crash in #10959